Forbid SHA-1 digests as part of RFC 9904 changes#3069
Conversation
3196c22 to
52a2031
Compare
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 2 comments.
Reviewable status: 0 of 25 files reviewed, 2 unresolved discussions (waiting on gbrodman).
core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java line 379 at r1 (raw file):
} public static boolean algorithmIsInvalid(int alg) {
Curious as to why this was inverted?
core/src/main/java/google/registry/tools/DigestType.java line 32 at r1 (raw file):
*/ public enum DigestType { // Algorithm number 1 is SHA-1 and is deliberately NOT SUPPORTED.
We need to lock this change behind a FeatureFlag and send out notification 30 days ahead of time.
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman made 2 comments.
Reviewable status: 0 of 25 files reviewed, 2 unresolved discussions (waiting on CydeWeys).
core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java line 379 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Curious as to why this was inverted?
Suggested by IDEA, because all calls to it were immediately inverted.
core/src/main/java/google/registry/tools/DigestType.java line 32 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
We need to lock this change behind a
FeatureFlagand send out notification 30 days ahead of time.
Done. We'll use the same feature flag for the algorithms too.
CydeWeys
left a comment
There was a problem hiding this comment.
Geez that's a lot of test changes ... too bad we were using a SHA1 digest everywhere.
@CydeWeys made 2 comments and resolved 2 discussions.
Reviewable status: 0 of 28 files reviewed, 1 unresolved discussion (waiting on gbrodman).
core/src/main/java/google/registry/tools/DigestType.java line 45 at r2 (raw file):
private final int wireValue; private final int bytes;
You could consider adding a third field here, something like boolean allowedInRfc, which would then make it easier to see in the constructor calls above what's allowed and what's not, and then the filter at the call site would be easier as it'd just call filter on the getter.
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys reviewed 28 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on gbrodman).
We can't change digest types that are already in the database but that's fine (since we just store them as integers). But we forbid them as part of domain creates/updates.
gbrodman
left a comment
There was a problem hiding this comment.
no kidding :(
@gbrodman made 2 comments.
Reviewable status: 26 of 28 files reviewed, 1 unresolved discussion (waiting on CydeWeys).
core/src/main/java/google/registry/tools/DigestType.java line 45 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
You could consider adding a third field here, something like boolean allowedInRfc, which would then make it easier to see in the constructor calls above what's allowed and what's not, and then the filter at the call site would be easier as it'd just call filter on the getter.
yeah sure, we'll be doing something similar when forbidding insecure algorithms too
We can't change digest types that are already in the database but that's fine (since we just store them as integers). But we forbid them as part of domain creates/updates.
This change is